Add Privacy Request Input Sanitization #2655

ThomasLaPiana · 2023-02-21T05:38:42Z

Closes https://github.com/ethyca/security-issues/issues/3
Closes https://github.com/ethyca/security-issues/issues/4

Code Changes

create a PhoneNumber class with validation that can be reused across the application
update phone number fields to use the new class
update email fields to use EmailStr instead of str
create a custom SafeStr class that sanitizes input values
add testing for new classes
Update the privacy request schemas to use SafeStr in place of str

Steps to Confirm

confirm that the above-linked issues have been fixed
poke around at other endpoints

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md

Description Of Changes

This PR adds new custom types designed to improve automatic validation and sanitization of user input data. It also applies these new custom types to the privacy request endpoints to remediate the above-linked issues

cypress · 2023-02-21T05:53:05Z

Passing run #813 ↗︎

0	3	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `5c1b583` into `cf7bcd2`...
Project: fides	Commit: `6e7d3938b4 ℹ️`
Status: Passed	Duration: 00:39 💡
Started: Mar 15, 2023 8:49 AM	Ended: Mar 15, 2023 8:50 AM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

codecov · 2023-02-21T06:00:05Z

Codecov Report

Patch coverage: 91.66% and project coverage change: +0.20 🎉

Comparison is base (5194405) 86.53% compared to head (5c1b583) 86.73%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2655      +/-   ##
==========================================
+ Coverage   86.53%   86.73%   +0.20%     
==========================================
  Files         291      295       +4     
  Lines       16488    16753     +265     
  Branches     2118     2145      +27     
==========================================
+ Hits        14268    14531     +263     
  Misses       1819     1819              
- Partials      401      403       +2

Impacted Files	Coverage Δ
src/fides/api/custom_types.py	`84.00% <84.00%> (ø)`
.../ops/api/v1/endpoints/consent_request_endpoints.py	`88.51% <100.00%> (+1.18%)`	⬆️
...ction_configuration/connection_secrets_bigquery.py	`100.00% <100.00%> (ø)`
...nnection_configuration/connection_secrets_email.py	`100.00% <100.00%> (ø)`
src/fides/api/ops/schemas/drp_privacy_request.py	`100.00% <100.00%> (ø)`
src/fides/api/ops/schemas/messaging/messaging.py	`97.79% <100.00%> (-0.05%)`	⬇️
src/fides/api/ops/schemas/privacy_request.py	`100.00% <100.00%> (ø)`
src/fides/api/ops/schemas/redis_cache.py	`100.00% <100.00%> (+10.52%)`	⬆️
src/fides/api/ops/schemas/registration.py	`100.00% <100.00%> (ø)`

... and 23 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ThomasLaPiana · 2023-02-21T16:15:25Z

@RobertKeyser @daveqnet would you mind giving me some examples here of clearly dangerous/invalid user input so I can use them for test cases? I trust y'all here as the experts 😄 thank you!

daveqnet · 2023-02-21T17:10:50Z

@RobertKeyser @daveqnet would you mind giving me some examples here of clearly dangerous/invalid user input so I can use them for test cases? I trust y'all here as the experts 😄 thank you!

Hmmm, it's a tricky one, as what constitutes dangerous input depends on what the input is being used for. Does the guidance here help: https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/07-Input_Validation_Testing/ ?

I'll also try to provide you a few examples of some real injection vulnerabilities from non-fides penetration tests to help.

NevilleS

Couple comments. But there's lots of other APIs to consider - here are a couple examples:

User.username, User.first_name, User.last_name
System.name, System.description (plus Dataset, DataCategory... all Resource models)
Organization has many string fields (name, description, controller.name...)
...?

src/fides/api/input_validation.py

src/fides/api/ops/schemas/connection_configuration/connection_secrets_email.py

pattisdr · 2023-02-24T15:53:31Z

If you're just looking for a list of our API endpoints that accept strings in the request body, I'd assume the majority do, too many to just list here.

Most of the ops schemas are here:
https://github.com/ethyca/fides/tree/main/src/fides/api/ops/schemas

If you're just trying to make a list to work from, you might write a script that goes through the contents of http://localhost:8080/openapi.json, for each request looks for the requestBody key and gets the schema from there - lots of recursion.

ThomasLaPiana · 2023-02-25T05:21:03Z

based on the feedback here, it sounds like the safest way to go about things here is to update every string to use InputStr and see where that lands us...

Additionally, logging around when values get truncated would be helpful for debugging

galvana · 2023-03-03T18:27:34Z

@ThomasLaPiana I know the unsafe tests aren't being run in Github but let's make sure we run those to make sure we don't break any connector integrations with this change.

src/fides/api/custom_types.py

ThomasLaPiana · 2023-03-10T08:06:20Z

quick update: I can't test the specific fixes I made because I'm not sure how to use the local test env. It now requires privacy request configurations. Will message in slack and try to get this sorted

an update to the update: Adam got me pointed in the right direction, I wasn't leveraging the special .env properly

ThomasLaPiana · 2023-03-15T16:52:03Z

Both security issues confirmed closed. Cleaning up now and merging

ThomasLaPiana · 2023-03-15T17:00:03Z

Patch coverage is lacking but overall coverage is up, we're calling this one a win 🙃

Failing tests are known-failing, considering this good to merge

Create input_validation.py

67a34da

ThomasLaPiana self-assigned this Feb 21, 2023

ThomasLaPiana added 8 commits February 23, 2023 21:58

feat: add email and phone number validation where needed

1bba355

fix: mypy issue

ff2b35c

feat: add big list of dangerous strings to sanitization test

674897c

feat: add tests for the PhoneNumber class

35018f1

fix: twilio error message test

c071c09

feat: add tests for the EmailSchema

164272e

fix: errant type change

ccf4921

fix: pylint

f13975b

NevilleS reviewed Feb 24, 2023

View reviewed changes

src/fides/api/input_validation.py Outdated Show resolved Hide resolved

pattisdr reviewed Feb 24, 2023

View reviewed changes

src/fides/api/ops/schemas/connection_configuration/connection_secrets_email.py Show resolved Hide resolved

ThomasLaPiana added 2 commits February 27, 2023 11:32

feat: add nox -s shell to quickly open a shell in a running webserver

fb61186

refactor: update names for the new safe types

30589f3

galvana reviewed Mar 3, 2023

View reviewed changes

src/fides/api/custom_types.py Show resolved Hide resolved

ThomasLaPiana added 2 commits March 10, 2023 12:42

feat: convert str to SafeStr for privacy requests

a1d39de

Merge branch 'main' into ThomasLaPiana-add-user-validation-sanitization

edbb4e8

ThomasLaPiana changed the title ~~Add broad server-side user input sanitization/validation~~ Add Privacy Request Input Sanitization Mar 10, 2023

ThomasLaPiana added 3 commits March 10, 2023 16:23

fix: static checks

0fa47a5

fix: allow phone numbers to be empty

4c6338a

Merge branch 'main' into ThomasLaPiana-add-user-validation-sanitization

88f41ea

ThomasLaPiana added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Mar 13, 2023

docs: changelog

519288c

ThomasLaPiana marked this pull request as ready for review March 14, 2023 13:39

ThomasLaPiana requested review from daveqnet and RobertKeyser March 14, 2023 13:40

fix: make the privacy request reason a safestr

5c1b583

ThomasLaPiana merged commit f6cfa4a into main Mar 15, 2023

ThomasLaPiana deleted the ThomasLaPiana-add-user-validation-sanitization branch March 15, 2023 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Privacy Request Input Sanitization #2655

Add Privacy Request Input Sanitization #2655

ThomasLaPiana commented Feb 21, 2023 •

edited

Loading

cypress bot commented Feb 21, 2023 •

edited

Loading

codecov bot commented Feb 21, 2023 •

edited

Loading

ThomasLaPiana commented Feb 21, 2023

daveqnet commented Feb 21, 2023

NevilleS left a comment •

edited

Loading

pattisdr commented Feb 24, 2023

ThomasLaPiana commented Feb 25, 2023

galvana commented Mar 3, 2023

ThomasLaPiana commented Mar 10, 2023 •

edited

Loading

ThomasLaPiana commented Mar 15, 2023

ThomasLaPiana commented Mar 15, 2023 •

edited

Loading

Add Privacy Request Input Sanitization #2655

Add Privacy Request Input Sanitization #2655

Conversation

ThomasLaPiana commented Feb 21, 2023 • edited Loading

Code Changes

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

cypress bot commented Feb 21, 2023 • edited Loading

Passing run #813 ↗︎

codecov bot commented Feb 21, 2023 • edited Loading

Codecov Report

ThomasLaPiana commented Feb 21, 2023

daveqnet commented Feb 21, 2023

NevilleS left a comment • edited Loading

Choose a reason for hiding this comment

pattisdr commented Feb 24, 2023

ThomasLaPiana commented Feb 25, 2023

galvana commented Mar 3, 2023

ThomasLaPiana commented Mar 10, 2023 • edited Loading

ThomasLaPiana commented Mar 15, 2023

ThomasLaPiana commented Mar 15, 2023 • edited Loading

ThomasLaPiana commented Feb 21, 2023 •

edited

Loading

cypress bot commented Feb 21, 2023 •

edited

Loading

codecov bot commented Feb 21, 2023 •

edited

Loading

NevilleS left a comment •

edited

Loading

ThomasLaPiana commented Mar 10, 2023 •

edited

Loading

ThomasLaPiana commented Mar 15, 2023 •

edited

Loading